Polish analysis of a multiclosure test #1982

comane · 2024-03-05T16:17:13Z

Here we collect new functions and template that allow the analysis of multiclosure tests.
@comane @giovannidecrescenzo @mariaubiali @andreab1997

validphys2/src/validphys/kinematics.py

comane

I think that the PR is ready for review.
The only things I would change as commented above is that the single data point ratio bias variance analysis does not reside in kinematics.py but rather in multiclosure_inconsistent.py / multiclosure_inconsistent_output.py

andreab1997 · 2024-03-18T10:10:03Z

@comane thanks for this. If you want, ask myself for the review (but also add one between @scarlehoff and @RoyStegeman given that part of this PR is based on my work and it does not make sense for me to review my own functions )

comane

Ciao @andreab1997 , I added some minor comments on some of the parts of the code that you and Giovanni wrote.

When you have the time, feel free to add a review to the inconsistent_closuretest folder

validphys2/src/validphys/scripts/vp_multiclosure.py

validphys2/src/validphys/compareinconsistentclosuretemplates/multiclosure_analysis.yaml

validphys2/src/validphys/kinematics.py

validphys2/src/validphys/closuretest/multiclosure.py

validphys2/src/validphys/closuretest/multiclosure_output.py

validphys2/examples/pca_bias_variance_report.yaml

validphys2/src/validphys/closuretest/inconsistent_closuretest/multiclosure_inconsistent.py

...phys2/src/validphys/closuretest/inconsistent_closuretest/multiclosure_inconsistent_output.py

validphys2/src/validphys/closuretest/multiclosure_output.py

validphys2/src/validphys/kinematics.py

scarlehoff · 2024-07-22T07:59:06Z

This is then ready to be merged? (@andreab1997 @comane )

andreab1997 · 2024-07-24T08:08:46Z

This is then ready to be merged? (@andreab1997 @comane )

I don't think so, surely I need to review this again but in any case I would wait for the paper to be published.

scarlehoff · 2024-07-24T08:18:15Z

This piece is complete and already rebased on top of master isn't it? If so it should be merged, worst case scenario you can note down the checksum of the commit but leaving it as a branch risks nobody taking care of it after the paper is out (which is what happened with the previous closure test branches and basically meant redoing a lot of stuff that mwilson already did)

andreab1997 · 2024-07-24T08:21:23Z

This piece is complete and already rebased on top of master isn't it? If so it should be merged, worst case scenario you can note down the checksum of the commit but leaving it as a branch risks nobody taking care of it after the paper is out (which is what happened with the previous closure test branches and basically meant redoing a lot of stuff that mwilson already did)

Ok I agree but still I would like to have another look. I will do before the end of this week in such a way we can merge this before saturday. Is that ok?

andreab1997

Ok I had a look and more or less nothing relevant changed since the last time I reviewed. @comane if you can answer the comments I left last time we can probably merge this soon.

comane · 2024-07-24T10:36:34Z

Hi @andreab1997, I addressed most of the comments that you left, as well as those that I wrote myself.

There is one main issue with this PR at present, namely, the way in which the compare inconsistent closure test script works.

This script is problematic, because it's not possible to have a dataset_inputs which is different from from_: fit, note that this is a problem since we want to have out of sample datasets.

The problem, I think, can be solved by removing the vp-compare fits from compareinconsistentclosuretemplates.
We don't need to have another vp-comparefits anyways.

If you could take care of this that would be great.

andreab1997 · 2024-07-24T10:42:47Z

Hi @andreab1997, I addressed most of the comments that you left, as well as those that I wrote myself.

There is one main issue with this PR at present, namely, the way in which the compare inconsistent closure test script works.

This script is problematic, because it's not possible to have a dataset_inputs which is different from from_: fit, note that this is a problem since we want to have out of sample datasets.

The problem, I think, can be solved by removing the vp-compare fits from compareinconsistentclosuretemplates. We don't need to have another vp-comparefits anyways.

If you could take care of this that would be great.

Just to understand, this issue is only there if you use the CLI or even if you write your own runcard and template?

comane · 2024-07-24T10:50:18Z

Just to understand, this issue is only there if you use the CLI or even if you write your own runcard and template?

I think it's there if I run validphys multiclosure_analysis.yaml (with dataset inputs that is not from the fit)

into 240305_multict_analysis

scarlehoff

Thanks for this, let's wait for the tests to finish then merge!

comane force-pushed the 240305_multict_analysis branch 4 times, most recently from 06db65c to b2b8557 Compare March 12, 2024 10:10

comane force-pushed the 240305_multict_analysis branch 2 times, most recently from bf843e1 to 46bba4f Compare March 17, 2024 16:57

comane commented Mar 17, 2024

View reviewed changes

validphys2/src/validphys/kinematics.py Outdated Show resolved Hide resolved

comane commented Mar 17, 2024

View reviewed changes

comane requested review from scarlehoff, RoyStegeman and andreab1997 March 19, 2024 20:22

comane force-pushed the 240305_multict_analysis branch from 5fca583 to 91d493d Compare April 11, 2024 21:35

comane force-pushed the 240305_multict_analysis branch 2 times, most recently from 509a521 to a09d394 Compare April 24, 2024 16:31

comane commented Apr 24, 2024

View reviewed changes

andreab1997 previously requested changes May 7, 2024

View reviewed changes

comane force-pushed the 240305_multict_analysis branch 2 times, most recently from b0ec210 to 59e3eea Compare June 11, 2024 16:00

comane force-pushed the 240305_multict_analysis branch from d6c0929 to 76001f4 Compare June 27, 2024 13:42

scarlehoff added the closure tests label Jul 17, 2024

comane force-pushed the 240305_multict_analysis branch from 76001f4 to cc5f8ac Compare July 17, 2024 14:24

andreab1997 reviewed Jul 24, 2024

View reviewed changes

comane added 23 commits October 22, 2024 13:21

added definition of delta in line with eq. 2.22

2b89c63

added definition of delta in line with eq. 2.22

998c652

added rbv scan for full dataset

e56cfce

added bootstrapped xi indicator function for full dataset

b4153a3

removed unused import from vp_multiclosure.py script

38164d1

removed new lines

a659461

removed unused variables

4fd4523

_covmats as array instead of list of arrays

1a912ab

added eigendecomposition function

1f2912b

removed compareinconsistent closure templates

c6c0e07

removed vp_multiclosure module

366cd03

report template for multiclosure inconsistent tests

95270ed

removed vp-multiclosure from pyproject

153c2c6

added and removed extra lines

308f528

removed extra line from congig

403944f

polished multiclosure.fits_normed_dataset_central_delta function

1407fd3

added meaningful description to single data point mapper

d567175

update author to lazy person

1939417

loop over reps

ab2cbd3

Lazy Person

7cc4eec

removed multiclosure_inconsistent and moved to multiclosure

7f8423f

removed multiclosure_inconsistent from __init__ file

403e354

addressed revision comments

d83d620

comane force-pushed the 240305_multict_analysis branch from 4c5c612 to d83d620 Compare October 22, 2024 12:21

comane added 3 commits October 22, 2024 13:49

avoid rewriting columns twice

6356c74

Merge branch '240305_multict_analysis' of https://github.com/NNPDF/nnpdf

12ba99e

into 240305_multict_analysis

avoid double column definition

6c4558b

scarlehoff approved these changes Oct 22, 2024

View reviewed changes

comane merged commit e66d753 into master Oct 22, 2024
6 checks passed

comane deleted the 240305_multict_analysis branch October 22, 2024 19:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Polish analysis of a multiclosure test #1982

Polish analysis of a multiclosure test #1982

comane commented Mar 5, 2024

comane left a comment

andreab1997 commented Mar 18, 2024

comane left a comment

scarlehoff commented Jul 22, 2024

andreab1997 commented Jul 24, 2024

scarlehoff commented Jul 24, 2024

andreab1997 commented Jul 24, 2024

andreab1997 left a comment

comane commented Jul 24, 2024

andreab1997 commented Jul 24, 2024 •

edited by comane

Loading

comane commented Jul 24, 2024

scarlehoff left a comment

Polish analysis of a multiclosure test #1982

Polish analysis of a multiclosure test #1982

Conversation

comane commented Mar 5, 2024

comane left a comment

Choose a reason for hiding this comment

andreab1997 commented Mar 18, 2024

comane left a comment

Choose a reason for hiding this comment

scarlehoff commented Jul 22, 2024

andreab1997 commented Jul 24, 2024

scarlehoff commented Jul 24, 2024

andreab1997 commented Jul 24, 2024

andreab1997 left a comment

Choose a reason for hiding this comment

comane commented Jul 24, 2024

andreab1997 commented Jul 24, 2024 • edited by comane Loading

comane commented Jul 24, 2024

scarlehoff left a comment

Choose a reason for hiding this comment

andreab1997 commented Jul 24, 2024 •

edited by comane

Loading